Skip to content

Fix chat commands#226

Open
FoofooTheGuy wants to merge 5 commits intomasterfrom
fix-chat-commands
Open

Fix chat commands#226
FoofooTheGuy wants to merge 5 commits intomasterfrom
fix-chat-commands

Conversation

@FoofooTheGuy
Copy link
Collaborator

fix my previous edits to clear message when there is an invalid ID
also fix my commenting style to match this project

void Chat::CommandLoop() {

//macro to clean up
#define CLEAR_RET \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good idea. It hides the return in the other lines, making the function less readable, the user won't know if the function returns there unless they look at the macro.
And performance wise a macro is useless, its contents get pasted into its declarations while compiling. At the end, it just makes the code less readable. Readability is very important. But I do have to admit that this function is not very clean. Maybe using lambdas might make it cleaner, it does go in a similar direction as your macro

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do something like this:

auto notifyAndClear = [this](TextID id) {
 OSDExtras::Notify(id, Color::Red);
 this->ClearPlayerMessage();
};

And then just do:

if (!IDList::AnimationValid(animID, GetPlayerIndex())) {
 return notifyAndClear(TextID::CHAT_INVALID_ANIMATION);
}

This would make it clear what is happening

Copy link
Collaborator Author

@FoofooTheGuy FoofooTheGuy Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, no disgustingly ugly macro definitions allowed here 👍
tbh, I thought the macro was ugly too but I didn't want to have duplicate code. A lambda indeed seems more like a c++ism. Thanks.

@FoofooTheGuy
Copy link
Collaborator Author

Here I have cleaned the function to hopefully make it more readable. each way to set ID has been moved to its own function and we don't even need to put ClearPlayerMessage everywhere anymore

return false;
}
ItemCommand();
return false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am guessing this was by accident? Should be return true;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep that’s a mistake 😓

@FoofooTheGuy
Copy link
Collaborator Author

Here I fixed that return.
I realize now why I didn't notice, because none of the returns are actually acknowledged yet... Still a mistake nonetheless so it's good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants